bugfix(particlesys): Delay creation of particle emitters until ParticleSystemManager is xfer-loaded#2333
Conversation
stephanmeesters
force-pushed
the
bugfix-nonsavable-emitters-xfer
branch
from
7418b31 to
16135b2
Compare
|
| Filename | Overview |
|---|---|
| Core/GameEngine/Source/GameClient/System/ParticleSys.cpp | Adds a DEBUG_ASSERTCRASH to the xfer-load path to catch particle systems created before the manager has a chance to restore its state — the central guard that motivates the whole PR. |
| Core/GameEngineDevice/Source/W3DDevice/GameClient/Drawable/Draw/W3DTankDraw.cpp | Moves tread-emitter creation from constructor to first draw call; refactors common logic into a new static helper, but the helper is duplicated in W3DTankTruckDraw.cpp and the original static_assert safety check was removed during loop unrolling. |
| Core/GameEngineDevice/Source/W3DDevice/GameClient/Drawable/Draw/W3DTankTruckDraw.cpp | Same structural changes as W3DTankDraw.cpp: deferred tread-emitter creation and identical static helper; the same duplication and lost static_assert issues apply. |
| GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Behavior/AutoHealBehavior.cpp | Particle system creation extracted to createEmitters(), guarded by INVALID_PARTICLE_SYSTEM_ID check and called lazily from update() and loadPostProcess(); correctly addresses the save-load race described in the PR. |
| GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Behavior/GrantStealthBehavior.cpp | Same deferred-creation pattern as AutoHealBehavior; createEmitters() is properly guarded and called from both update() and loadPostProcess(). |
| GeneralsMD/Code/GameEngine/Include/GameLogic/Module/AutoHealBehavior.h | Adds createEmitters() private method declaration; minor cosmetic brace-style change on the class definition. |
| GeneralsMD/Code/GameEngine/Include/GameLogic/Module/GrantStealthBehavior.h | Adds createEmitters() private method declaration alongside m_radiusParticleSystemID. |
Sequence Diagram
sequenceDiagram
participant L as SaveGame Loader
participant PSM as ParticleSystemManager
participant OBJ as GameObject (e.g. Tank)
participant DRAW as W3DTankDraw
participant BEH as AutoHealBehavior
Note over L,BEH: Save-game load sequence (fixed)
L->>OBJ: construct object
OBJ->>DRAW: constructor (no emitters created)
OBJ->>BEH: constructor (no emitters created)
L->>PSM: xfer() — restores particle systems from save
PSM-->>L: m_systemMap fully populated
L->>DRAW: loadPostProcess()
DRAW->>PSM: createTreadEmitters() [safe, PSM ready]
L->>BEH: loadPostProcess()
BEH->>PSM: createEmitters() [safe, PSM ready]
Note over DRAW,PSM: During normal gameplay (non-save)
DRAW->>PSM: createTreadEmitters() on first doDrawModule()
BEH->>PSM: createEmitters() on first update()
Prompt To Fix All With AI
This is a comment left during a code review.
Path: Core/GameEngineDevice/Source/W3DDevice/GameClient/Drawable/Draw/W3DTankDraw.cpp
Line: 128-142
Comment:
**Duplicate static helper duplicated across translation units**
The `createParticleSystem` static helper function is defined identically in both `W3DTankDraw.cpp` (lines 128–142) and `W3DTankTruckDraw.cpp` (lines 184–198). Any future bug fix or behavioural change needs to be applied in two places. Consider extracting it to a shared header or a common utility source file (e.g. a `W3DParticleUtils` inline/static function) that both TUs include.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: Core/GameEngineDevice/Source/W3DDevice/GameClient/Drawable/Draw/W3DTankDraw.cpp
Line: 144-157
Comment:
**`static_assert` array-size check removed by loop unrolling**
The original loop used a `static_assert` to guarantee that `treadDebrisNames` and `m_treadDebrisIDs` have the same number of elements. Unrolling the loop into two separate `if`-blocks silently dropped this compile-time safety net. If `MAX_TREAD_IDS` (or the underlying array) is ever extended to three or more entries, the third slot will never be populated and the mismatch will go undetected.
The same `static_assert` pattern is still preserved in `W3DTankTruckDraw::createWheelEmitters()`, so restoring it here would be consistent:
```cpp
void W3DTankDraw::createTreadEmitters()
{
if (getW3DTankDrawModuleData())
{
static_assert(ARRAY_SIZE(m_treadDebrisIDs) == 2, "Update createTreadEmitters if array size changes");
if (m_treadDebrisIDs[0] == INVALID_PARTICLE_SYSTEM_ID)
m_treadDebrisIDs[0] = createParticleSystem(getW3DTankDrawModuleData()->m_treadDebrisNameLeft, getDrawable());
if (m_treadDebrisIDs[1] == INVALID_PARTICLE_SYSTEM_ID)
m_treadDebrisIDs[1] = createParticleSystem(getW3DTankDrawModuleData()->m_treadDebrisNameRight, getDrawable());
}
}
```
The same applies to `W3DTankTruckDraw::createTreadEmitters()` at the equivalent location.
How can I resolve this? If you propose a fix, please make it concise.Reviews (7): Last reviewed commit: "Add assert in ParticleSystemManager" | Re-trigger Greptile
stephanmeesters
force-pushed
the
bugfix-nonsavable-emitters-xfer
branch
from
16135b2 to
7b4b2fd
Compare
stephanmeesters
changed the title
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Behavior/AutoHealBehavior.cpp
Outdated
Show resolved
Hide resolved
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Behavior/GrantStealthBehavior.cpp
Outdated
Show resolved
Hide resolved
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Behavior/AutoHealBehavior.cpp
Outdated
Show resolved
Hide resolved
| createTreadEmitters(); | ||
| // TheSuperHackers @bugfix stephanmeesters 20/02/2026 | ||
| // If loading from savegame, delay non-saveable emitter creation until postProcess. | ||
| if (TheGameState == nullptr || TheGameState->isInLoadGame() == FALSE) |
There was a problem hiding this comment.
This is calling a function that is commented with "brutal hack", indicating that using this function is undesired. Can we solve this another way perhaps? Perhaps create particle effects later?
There was a problem hiding this comment.
Particle effects are now created later, in update or doDrawModule. When loading from savegame these happen after loadPostProcess.
I've reworked the create emitters functions a bit to be more performant and readable
There was a problem hiding this comment.
So my first thought here is that this is a workaround for a limitation of the particle system manager; changing the particle creation on object creation to a lazy update creation. This is similar to what the wheel emitters do. However, it does not fix the root issue with the particle system manager. So if someone created particles on object creation in the future again, then the issue returns. Is there a way to make the particle system manager robust against this? Or at least add an assert that signals that it is invalid?
There was a problem hiding this comment.
I've added an assert in ParticleSystemManager::xfer that tests that the list is empty on load, this seems like a good thing to have.
One thing we can do is move CHUNK_ParticleSystem before CHUNK_GameLogic, which means the particle system manager is xfer-loaded early. I verified that this will fix the order of adding particle systems. This only applies to new saves as old saves load the chunks in the order it was saved.
However adding particle systems in the constructor will be problematic regardless of chunk order in the case of a savegame: in the old code there was tossTreadEmitters() in postProcess(), which appears to be needed because otherwise these early created particles are no longer visible for some reason.
So doing the lazy initalization seems like a good thing. Moving the chunks around would hide this particular problem (and may cause other issues idk).
stephanmeesters
changed the title
stephanmeesters
changed the title
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
…toHealBehavior.cpp Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
stephanmeesters
force-pushed
the
bugfix-nonsavable-emitters-xfer
branch
from
ee59fc0 to
1f55acd
Compare
Core/GameEngineDevice/Source/W3DDevice/GameClient/Drawable/Draw/W3DTankTruckDraw.cpp
Show resolved
Hide resolved
2 participants
During loading a savegame, some particle systems were created before
ParticleSystemManagerhad the opportunity to xfer-load the saved particle systems, which led to the possibility that some of these early created particle systems could be overwritten inm_systemMapofParticleSystemManager. When that happens they would no longer reliably be able to use master/slave systems. The retail game does not use master/slave on the involved effects but it could appear as an issue with some mods (as the bug report describes too).Misc findings
W3DTankDrawandW3DTankTruckDrawwould create particle systems in the constructor, only to purge and recreate them again inpostProcess, this has been changed to look if we're loading a savegame and then only create it once.GrantStealthBehaviortriggers when you do a GPS scrambler, but the behavior and associated particle effect will only last for one frame. The particle effect references a texture that's available in Generals but not in Zero Hour. So I think that so far nobody has been able to see this particle effect in action.Todo